Skip to content

Fix TCP Requester #recv_reply #105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
st0012 merged 2 commits into from
Dec 18, 2025
Merged

Fix TCP Requester #recv_reply #105

st0012 merged 2 commits into from
Dec 18, 2025

Conversation

@jsaubry
Copy link
Contributor

@jsaubry jsaubry commented Oct 29, 2025

I've been seeing some intermittent NoMethodError undefined method 'unpack' for nil exceptions raised from Resolv.

NoMethodError undefined method 'unpack' for nil
/ruby/3.4.0/resolv.rb:931 Resolv::DNS::Requester::TCP#recv_reply
/ruby/3.4.0/resolv.rb:720 Resolv::DNS::Requester#request
/ruby/3.4.0/resolv.rb:558 block in Resolv::DNS#fetch_resource
/ruby/3.4.0/resolv.rb:1168 block (3 levels) in Resolv::DNS::Config#resolv
/ruby/3.4.0/resolv.rb:1166 Array#each
/ruby/3.4.0/resolv.rb:1166 block (2 levels) in Resolv::DNS::Config#resolv
/ruby/3.4.0/resolv.rb:1165 Array#each
/ruby/3.4.0/resolv.rb:1165 block in Resolv::DNS::Config#resolv
/ruby/3.4.0/resolv.rb:1163 Array#each
/ruby/3.4.0/resolv.rb:1163 Resolv::DNS::Config#resolv
/ruby/3.4.0/resolv.rb:540 Resolv::DNS#fetch_resource
/ruby/3.4.0/resolv.rb:523 Resolv::DNS#each_resource
/ruby/3.4.0/resolv.rb:514 Resolv::DNS#getresources

This seems to have been caused by misbehaving DNS servers closing connections before answering. The fix is to simply make sure we were able to read data from the server before trying to interpret it.

The raised Errno::ECONNRESET will be caught by the calling #request method and re-raised as ResolvTimeout.

@jsaubry
Copy link
Contributor Author

jsaubry commented Nov 17, 2025 •
edited
Loading

@hsbt I would love to get a review on this

rhenium
rhenium reviewed Nov 17, 2025
View reviewed changes
lib/resolv.rb Outdated
raise Errno::ECONNRESET if len_data.nil?
len = len_data.unpack('n')[0]
reply = @socks[0].read(len)
raise Errno::ECONNRESET if reply.nil?
Copy link
Member

@rhenium rhenium Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, could we also fix the cases where len_data.bytesize != 2 and reply.bytesize != len?

Raising Errno::ECONNRESET when it doesn't involve the ECONNRESET errno feels wrong.

Copy link
Contributor Author

@jsaubry jsaubry Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment! You are right that ECONNRESET was probably not the right exception. I changed it to EOFError which is probably not perfect, but much closer to reality.

@jsaubry jsaubry requested a review from rhenium December 5, 2025 12:48
@st0012 st0012 added the bug Something isn't working label Dec 8, 2025
st0012
st0012 approved these changes Dec 8, 2025
View reviewed changes
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits on newlines. Otherwise looks good to me 👍
@rhenium Would you mind giving it another look? Thanks

@jsaubry jsaubry force-pushed the fix-tcp-recv_reply branch from 5d37435 to 9c640bd Compare December 8, 2025 16:23
@st0012 st0012 merged commit 83246e7 into ruby:master Dec 18, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@st0012 st0012 st0012 approved these changes

@rhenium rhenium Awaiting requested review from rhenium

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jsaubry @rhenium @st0012